Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update acorn, fix tests #883

Merged
merged 2 commits into from
Apr 10, 2024
Merged

Update acorn, fix tests #883

merged 2 commits into from
Apr 10, 2024

Conversation

jsg2021
Copy link
Contributor

@jsg2021 jsg2021 commented Jul 8, 2020

Updated: Updating acorn adds support for the language features users have been requesting. It works. However, running the tests against the current-generation node is proving challenging. All tests pass on node 11 (at least locally), on node 12+ things break subtly. There is one test that is probably a legitimate failure. Mocha is trying to diff a "module" and its function to normalize values doesn't have a case for 'module' and is defaulting to coercing to a string... which blows up. I'm not sure when the "module" object type was hardened, but that caused moduleObject + '' to throw a type error. Also, node 12 seems to have changed how you hook into --check or removed it... because those tests are failing only in node 12 and up.

The remainder of the failures are from running the tests on node 12 or newer. Dependencies that have type: module in their package.json brake the current usage of require(thing).

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 8, 2020

This resolves #879 and #866

@hroland
Copy link

hroland commented Jul 10, 2020

I can't seem to get this fork to work, neither with node -r esm, nor with initializing via main.js and index.js :(

Always hitting this error, even tried a fresh project:

<project path>/esm-test/node_modules/esm/esm.js:156
  const { dir } = shared.package
          ^

TypeError: Cannot destructure property 'dir' of 'shared.package' as it is undefined.
    at Object.<anonymous> (/<project path>/node_modules/esm/esm.js:156:11)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
    at Module.load (internal/modules/cjs/loader.js:1002:32)
    at Function.Module._load (internal/modules/cjs/loader.js:901:14)
    at Module.require (internal/modules/cjs/loader.js:1044:19)
    at require (internal/modules/cjs/helpers.js:77:18)
    at Object.<anonymous> (/<project path>/index.js:3:11)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)

Any ideas maybe?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 11, 2020

I ran the build script then tested. npm run build?

@matthewharwood
Copy link

@jsg2021 Any update on this?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 21, 2020

I have not received any feedback or help.

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 22, 2020

@jdalton I'm willing to help with this, but I need some guidance on resolving the test failures.

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 22, 2020

Just tested master, and these same tests fail. Running tests on v3.2.25, has simular failures. (--check tests, and pm2 scenarios)

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 22, 2020

I've fixed all errors on node 11. There still are errors on node 12+. Looks like Node12+ broke the --check support. :/ The other failures are from attempting to require() a "module" and node is blowing up saying to use import().

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 22, 2020

Locally on node 11, using npm ci to install deps, tests all pass. The tests that are failing on CI and I'm not sure why.

@jsg2021 jsg2021 changed the title Update acorn. (add nullish and optional chain syntax support) Update acorn. Jul 24, 2020
@jsg2021 jsg2021 changed the title Update acorn. Update acorn, fix tests Jul 24, 2020
@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 24, 2020

I've worked through many of the failures. Should be good to go.

Side thought... the tests run on node configured in the build matrix. However, it also builds on those same instances. With newer dev deps cutting off at 10, it would probably be better to isolate the build and tests. So we can validate the compiled module works on node 6+ and not be required to build on less than node 10.

@hroland
Copy link

hroland commented Jul 24, 2020

Great work, I hope it gets merged soon!

The reason that caused my error was when I added the package to an existing project via yarn add jsg2021/esm --latest, the build script didn't run. After cd ./node_modules/esm && yarn && yarn build, running node -r esm . worked!

Any thoughts why build doesn't run after adding the package?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 24, 2020 via email

@antony
Copy link

antony commented Jul 28, 2020

This is great - thanks for your hard work.

One thing I noticed is that linting isn't passing any more.

/home/runner/work/esm/esm/src/hook/vm.js
##[error]  240:17  error  Setter cannot return a value  no-setter-return
##[error]  247:13  error  Setter cannot return a value  no-setter-return

/home/runner/work/esm/esm/test/fixture/import/const.mjs
##[error]  3:1  error  'value' is read-only  no-import-assign

/home/runner/work/esm/esm/test/fixture/import/let.mjs
##[error]  3:1  error  'value' is read-only  no-import-assign

✖ 4 problems (4 errors, 0 warnings)

This would probably need to be fixed in order for this to be merged?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 28, 2020

is lint not run as a check?

@antony
Copy link

antony commented Jul 28, 2020

It doesn't look like it, from studying the travis and appveyor config, it's omitted. I believe it is run by husky on the author's machine before push.

However I run it on ci for consistency, and it makes the build fail: https://github.com/beyonk-adventures/esm/actions/runs/185440336

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 28, 2020

I believe the errors in the test/fixtures files are intentional.

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 28, 2020

And double checked, and I haven't modified those files.

@antony
Copy link

antony commented Jul 28, 2020

Sorry, sent you the wrong one: https://github.com/beyonk-adventures/esm/runs/918092904?check_suite_focus=true

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 28, 2020

It doesn't look like it, from studying the travis and appveyor config, it's omitted. I believe it is run by husky on the author's machine before push.

However I run it on ci for consistency, and it makes the build fail: https://github.com/beyonk-adventures/esm/actions/runs/185440336

The error in that run is the EPRIVATE refusing to publish private package? The stacktrace is just a Module Concatenation bailout notice. I'm not seeing the lint errors?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Jul 28, 2020

I would fix the lint errors, but i hesitate to include orthogonal changes.

@antony
Copy link

antony commented Jul 28, 2020

Perhaps they're not important and already existed. Just thought I'd highlight them in case they would block merge for the PR. Since CI doesn't appear to depend on them, w can leave it to the package owners to decide the importance I guess.

@hroland
Copy link

hroland commented Jul 28, 2020

the build doesn’t run because dev deps aren’t installed unless you run npm/yarn install from that package. esm is a zero-dep library. it must be built and published to be used.

Ahh, thanks for letting me know! I learned something today 🙂

@matthewharwood
Copy link

Any kind of ETA on this the suspense is killing me lol

@jsg2021
Copy link
Contributor Author

jsg2021 commented Aug 8, 2020

I haven't heard from any maintainer.:(

@jsg2021
Copy link
Contributor Author

jsg2021 commented Aug 9, 2020

squashed the test fixes.

You can use this PR directly until it updates by using a url as your version spec.

  1. Clone my fork
  2. run npm install and pack:
# This will produce a tarball `./esm-3.2.25.tgz` that you can point to...
$ npm install && npm pack
  1. Move the .tgz file to your project directory and rename to esm-3.x.x-pr883.tgz
  2. Set esm's version to file://./esm-3.x.x-pr883.tgz in your package.json and reinstall your node_modules.

Or if you trust public urls (I wouldn't but I'm offering for simplicity) you can set the version url to:

https://github.com/jsg2021/esm/releases/download/v3.x.x-pr883/esm-3.x.x-pr883.tgz

@hroland
Copy link

hroland commented Aug 11, 2020

Waiting eagerly :) @jdalton @benjamn

@Ghazay
Copy link

Ghazay commented Aug 20, 2020

Waiting <3

@galvez
Copy link

galvez commented Aug 25, 2020

Pinging @jdalton -- if you can find the time, we'll collectively owe you a beer.

@yurikoex
Copy link

squashed the test fixes.

You can use this PR directly until it updates by using a url as your version spec.

  1. Clone my fork
  2. run npm install and pack:
# This will produce a tarball `./esm-3.2.25.tgz` that you can point to...
$ npm install && npm pack
  1. Move the .tgz file to your project directory and rename to esm-3.x.x-pr883.tgz
  2. Set esm's version to file://./esm-3.x.x-pr883.tgz in your package.json and reinstall your node_modules.

Or if you trust public urls (I wouldn't but I'm offering for simplicity) you can set the version url to:

https://github.com/jsg2021/esm/releases/download/v3.x.x-pr883/esm-3.x.x-pr883.tgz

how would I activate optional chaining with this? Still seeing 'SyntaxError: Unexpected token '.''

@jsg2021
Copy link
Contributor Author

jsg2021 commented Aug 26, 2020

this doesn't enable syntax. It only prevents the library from blowing up if used on a version of node that supports it and the code uses it.

node 14 has it enabled by default. earlier versions have it behind a flag.

@daveisfera
Copy link

@jdalton With node 14 hitting LTS soon, any chance we can get this merged and released?

@jsg2021
Copy link
Contributor Author

jsg2021 commented Sep 30, 2020

@daveisfera It would be nice to have for sure. However, node 14 will ship with official first-pass esm support. It won't be a dropin replacement for webpack/rollup but you can largely write new code with its idioms today... and THOSE work in both.

@daveisfera
Copy link

Unfortunately, there are still some kinks being worked out and not all uses work yet (like this one)

@daveisfera
Copy link

Any chance of getting this merged since the built-in ESM support in node isn't quite as usable as esm is?

@hadyrashwan
Copy link

When can we expect this PR? as we use nullish coalescing in our code

@jsg2021
Copy link
Contributor Author

jsg2021 commented Feb 19, 2021

I don't have control. There are still failing tests (related to node 13/14/15 repl)... @jdalton is a busy man and is very likely burned out with this project. I'd consider this project dead. My fork is open, feel free to fix the outstanding tests and maybe eventually someone will be given maintainer privileges.

I'm planning to phase out this project in my code and use native esm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

10 participants